OCI format ADR#115
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR introduces two ADR documents that establish Lola's approach to OCI-based artifact distribution. The main ADR outlines the decision to support ChangesOCI Support Decision and CLI Specification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
docs/adr/0007-oci-format.md (2)
445-445: Use “CLI compatibility” instead of “CLI interface compatibility.”“Interface” is redundant in this phrase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0007-oci-format.md` at line 445, Update the phrasing in the ADR text by replacing the phrase "CLI interface compatibility (Click → Cobra)" with the concise "CLI compatibility (Click → Cobra)"; locate the exact string "CLI interface compatibility (Click → Cobra)" in docs/adr/0007-oci-format.md and change it to "CLI compatibility (Click → Cobra)" so the word "interface" is removed.
365-365: Add language identifiers to fenced code blocks.Fences opened on Line 365, Line 392, and Line 410 are missing a language tag, which triggers
MD040. Usingtextfor tree/architecture diagrams keeps lint clean and readability high.Proposed doc-only diff
-``` +```text OCI Image: ... -``` +``` -``` +```text .lola/modules/module-name/ ... -``` +``` -``` +```text lola (Go binary) ... -``` +```Also applies to: 392-392, 410-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0007-oci-format.md` at line 365, Three fenced code blocks are missing language identifiers causing MD040; update the three backtick fences that surround the blocks starting with "OCI Image:", ".lola/modules/module-name/", and "lola (Go binary)" to use a language tag (use "text") so each opening fence becomes ```text and the corresponding closing fence remains ```, keeping the block contents unchanged; apply the same change for the other occurrences mentioned (the blocks around those three headings).docs/adr/0007-oci-format/oci-cli-exploration.md (1)
29-29: Add a language tag to the fenced block at Line 29.This currently triggers
MD040;textis appropriate for the tree diagram.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0007-oci-format/oci-cli-exploration.md` at line 29, The fenced code block that contains the tree diagram is missing a language tag (triggering MD040); update the opening backtick fence for that block (the fenced block beginning at the tree diagram around line 29) to include the language tag "text" (e.g., ```text) so the markdown linter recognizes the code block type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0007-oci-format/oci-cli-exploration.md`:
- Around line 1441-1443: The documented provenance flag is inconsistent: replace
the mention of `--skip-provenance` with the already defined
`--skip-verification` (or explicitly document both if provenance has a separate
flag) so the "Provenance verification" bullet uses the same flag names as the
earlier OCI command options; update the line that reads `Provenance
verification: **Enabled** for OCI modules (use --skip-provenance flag to
disable)` to reference `--skip-verification` (or add clarifying text listing
both `--skip-verification` and `--skip-provenance`) to keep
`--skip-verification` and `--skip-provenance` consistent across the doc.
- Around line 909-913: The docs show conflicting command flows for installing
OCI modules (sometimes using the two-step "lola mod add <source>" then "lola
install <module> -a <assistant>" flow, and other times invoking "lola install
oci://..." directly, and option names like "--verify-signature" don't match
earlier specs); pick one canonical contract (either the two-step registry flow
using "lola mod add" + "lola install <module> -a <assistant>" or the
direct-install flow "lola install oci://<...> -a <assistant>"), then update all
examples and error messages to that contract consistently (including normalizing
option names such as "--verify-signature" to the agreed flag name), and audit
the referenced sections (the current examples and errors around the OCI CLI
exploration doc, plus the other occurrences noted) to ensure every example,
error text, and option usage matches the chosen command format and flag names
(search for "lola mod add", "lola install", "oci://", and "--verify-signature"
to find instances to change).
- Around line 27-28: The doc currently declares Lola modules are packaged as
"single-layer OCI images" but the example outputs later show multiple content
layers; update the examples that describe image build/push output (the examples
currently showing multiple content layers) to reflect a single-layer model by
consolidating layers into one content layer in the output text, adjusting
sizes/digests/summary lines accordingly, and ensuring any CLI outputs or example
manifests referenced in the "single-layer OCI images" section are consistent
with that single-layer representation; search for the phrase "single-layer OCI
images" and the example blocks that show "content layers" and change those
outputs to a single-layer format.
- Around line 73-80: The Table of Contents contains anchor fragments that don't
match existing headings (e.g., entries like "#new-commands",
"#modified-commands", "#configuration", "#possible-implementation-phasing");
update each TOC link to the exact heading text used in the document (or
normalize the headings to match the TOC) so intra-doc navigation works—ensure
the TOC entries such as "New Commands", "Modified Commands", "Configuration",
and "Possible Implementation Phasing" map exactly to their corresponding
headings or change the headings to the slugified forms used in the TOC so
anchors like Module Metadata and SkillCard and Workflow Pattern: Add Then
Install resolve correctly.
---
Nitpick comments:
In `@docs/adr/0007-oci-format.md`:
- Line 445: Update the phrasing in the ADR text by replacing the phrase "CLI
interface compatibility (Click → Cobra)" with the concise "CLI compatibility
(Click → Cobra)"; locate the exact string "CLI interface compatibility (Click →
Cobra)" in docs/adr/0007-oci-format.md and change it to "CLI compatibility
(Click → Cobra)" so the word "interface" is removed.
- Line 365: Three fenced code blocks are missing language identifiers causing
MD040; update the three backtick fences that surround the blocks starting with
"OCI Image:", ".lola/modules/module-name/", and "lola (Go binary)" to use a
language tag (use "text") so each opening fence becomes ```text and the
corresponding closing fence remains ```, keeping the block contents unchanged;
apply the same change for the other occurrences mentioned (the blocks around
those three headings).
In `@docs/adr/0007-oci-format/oci-cli-exploration.md`:
- Line 29: The fenced code block that contains the tree diagram is missing a
language tag (triggering MD040); update the opening backtick fence for that
block (the fenced block beginning at the tree diagram around line 29) to include
the language tag "text" (e.g., ```text) so the markdown linter recognizes the
code block type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c44a68ff-1a38-4923-8a24-bf49119de34e
📒 Files selected for processing (2)
docs/adr/0007-oci-format.mddocs/adr/0007-oci-format/oci-cli-exploration.md
There was a problem hiding this comment.
I love this idea so much and I think it goes great with the move to golang.
@mrbrandao have you read this yet?
@dmartinol my only feedback would be that you can probably reduce the length of this ADR
|
Oh and @dmartinol can you rebase and fix some of the issues like the table of contents? |
@SecKatie Yes, I read ;-) Lets do it. @dmartinol in the PR: #109 I'm accepting your proposal for ADR names. Please also rename the ADR to match the new name conventions. |
|
@mrbrandao @SecKatie |
SecKatie
left a comment
SecKatie
left a comment
There was a problem hiding this comment.
One nit. I think this is good to go in without that being addressed. @dmartinol let me know if you want this merged as is.
| err = artifact.ExtractTo("/path/to/.lola/modules/") // after verification succeeds | ||
| ``` | ||
|
|
||
| CLI command proposals (`lola build`, `lola push`, `lola sign`, `lola verify`, phasing, metadata schema) live in [OCI CLI Exploration](oci-format/oci-cli-exploration.md). |
There was a problem hiding this comment.
I feel like we should recommend cosign for signing. I think lola verify makes sense but I'm not sure we want the liability for our users if our signing doesn't work properly.
There was a problem hiding this comment.
same concern for lola sign, I imagine
There was a problem hiding this comment.
I think so. Do you want to hold this open and change that or is it okay and we can keep taking about it after this is merged?
There was a problem hiding this comment.
I think so. Do you want to hold this open and change that or is it okay and we can keep taking about it after this is merged?
I think it's better to review the low level details at execution time. So I'd merge this as-is for now.
There was a problem hiding this comment.
Can we merge it after the latest rebase?
|
Oh and @dmartinol can you rebase? Sorry for all the back and forth 😅 |
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
…OCI model. Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
…mentation Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
3 participants
Summary
An ADR to integrate OCI format for lola modules.
Related Issues
NA
Test Plan
AI Disclosure
AI-assisted with Claude Code
Summary by CodeRabbit
Documentation